PR Review: feat: support class-level CombinedDataSources with static and method data sources

Summary

This PR fixes issue #3990 by removing the IAccessesInstanceData marker interface from CombinedDataSourcesAttribute, allowing it to work at the class level with static data sources. The fix is elegant and correctly addresses the circular dependency problem that was preventing class-level usage.

✅ Positives

1. Correct Root Cause Fix: The removal of IAccessesInstanceData is the right approach. CombinedDataSourcesAttribute already has runtime checks (lines 156-162 in CombinedDataSourcesAttribute.cs) that detect and prevent instance data sources when no instance is available, making the interface marker unnecessary.

2. Good Test Coverage: The new tests in ClassLevelCombinedDataSourcesTests.cs cover multiple scenarios:

   • Static Arguments
   • Static MethodDataSource
   • Mixed class and method level
   • Multiple data source types
   • Three parameters (Cartesian product verification)

3. Proper Snapshot Updates: All public API snapshots were correctly updated across all target frameworks.

4. Clear Documentation: The test file includes helpful comments explaining expected test case counts (e.g., "3 × 2 = 6 test cases").

⚠️ Issues & Recommendations

1. CRITICAL: Dual-Mode Testing Missing (Violation of Rule 1)

TUnit's MANDATORY Rule #1: ALL changes must work identically in both source-generated mode AND reflection mode.

Issue: The new tests don't explicitly verify both execution modes. While the tests will run in source-gen mode (the default), there's no explicit verification that reflection mode works identically.

Recommendation: Add dual-mode test verification. Based on existing TUnit patterns, consider adding tests like:

// Example pattern to verify both modes
[Test]
[Arguments(ExecutionMode.SourceGenerated)]
[Arguments(ExecutionMode.Reflection)]
public async Task ClassLevel_WorksInBothModes(ExecutionMode mode)
{
    // Test logic that verifies both modes produce identical results
}

Or at minimum, verify manually that dotnet test with reflection mode enabled works correctly.

2. Source Generator Review Needed

Question: Does the source generator have any special handling for IAccessesInstanceData that might be affected by this change?

I searched for CombinedDataSources in the source generator code and found no matches, which is good—it suggests the source generator treats CombinedDataSourcesAttribute generically as an IAsyncUntypedDataSourceGeneratorAttribute.

Recommendation: Verify that class-level CombinedDataSources generates correct code. Check the generated files in:

obj/Debug/net9.0/generated/TUnit.Core.SourceGenerator/

3. Runtime Check Robustness

The existing runtime check (lines 156-162) is good, but the error message could be more helpful:

Current:

"Cannot use instance-based data source attribute on parameter '{parameterMetadata.Name}' when no instance is available. " +
"Consider using static data sources or ensure the test class is properly instantiated."

Suggestion: Add context about class-level vs method-level:

"Cannot use instance-based data source attribute on parameter '{parameterMetadata.Name}' when used at class level. " +
"When using [CombinedDataSources] at the class level (on constructor parameters), all data sources must be static. " +
"Use static MethodDataSource or Arguments attributes, or move [CombinedDataSources] to the method level to use instance data."

4. Test Count Verification

The tests have helpful comments about expected test counts, but they don't actually verify the count. Consider adding a test that verifies the correct number of test cases are generated (similar to VerifyCartesianProduct_TwoParameters in CombinedDataSourceTests.cs).

5. Edge Case: What About Instance MethodDataSource?

Question: What happens if someone tries this?

[CombinedDataSources]
public class MyClass(
    [InstanceMethodDataSource(nameof(GetData))] int x)  // Instance method!
{
    public IEnumerable<int> GetData() => [1, 2, 3];
    
    [Test]
    public void MyTest() { }
}

Expected: Should fail with a clear error message.
Recommendation: Add a test case to verify this error scenario is handled gracefully.

6. Minor: Code Style

The new test file follows TUnit conventions well. One minor note: consider using modern C# collection expressions consistently (you already do in most places):

// ✅ Already doing this
await Assert.That(x).IsIn([1, 2, 3]);

🧪 Testing Checklist (from CLAUDE.md)

Based on TUnit's pre-commit checklist:

• ✅ Public API changed → API snapshots updated
• ❌ Dual-mode feature → Missing explicit dual-mode tests
• ⚠️ Source generator verification needed (manual check recommended)
• ✅ Code follows style guide
• ✅ Good test coverage for happy path
• ⚠️ Missing tests for error scenarios

🎯 Recommendations Summary

Must Fix (before merge):

1. ✅ Verify or add explicit dual-mode testing (both source-gen and reflection)
2. ✅ Manually verify source-generated code is correct for class-level CombinedDataSources

Should Fix:
3. Improve error message for instance data source at class level
4. Add test for instance method data source error case

Nice to Have:
5. Add test count verification
6. Consider adding a test that mixes static and instance data sources to verify the error

🏁 Conclusion

This is a solid fix that correctly addresses the root cause. The implementation is clean, and the test coverage is good for the happy path.

Before merging, please:

1. Confirm both execution modes work (source-gen and reflection)
2. Manually inspect generated code to ensure it's correct

Once these are verified, this PR should be good to merge!

Great work fixing this issue! The approach of removing the unnecessary marker interface while keeping the runtime safety checks is exactly right. 👍

Pull Request Review - PR #4007

Summary

This PR successfully implements support for class-level CombinedDataSources (issue #3990) by removing the blanket IAccessesInstanceData interface from the attribute and adding runtime validation with improved error messaging.

✅ Strengths

1. Excellent Error Message Improvement ⭐

The enhanced error message in CombinedDataSourcesAttribute.cs:160-164 is outstanding:

• Clearly explains the problem (instance-based data source at class level)
• Identifies the specific attribute and parameter causing the issue
• Provides actionable solutions (use static sources OR move to method level)
• Much better than the previous "circular dependency" error

2. Comprehensive Test Coverage ✅

The test file demonstrates thorough testing across multiple scenarios:

• Static Arguments (Test 1)
• Static MethodDataSource (Test 2)
• Mixed class/method level (Test 3)
• Mixed data source types (Test 4)
• Three parameters (Test 5)
• Edge case documentation (helpful for future maintainers)

3. Correct API Change ✅

Removing IAccessesInstanceData from CombinedDataSourcesAttribute is the right approach:

• The interface was too broad and caused false positives
• Runtime check provides better context and error messages
• Snapshots correctly updated across all target frameworks

4. Code Style Compliance ✅

• Uses collection expressions: [] (line 171)
• Modern C# syntax throughout
• Follows TUnit conventions per CLAUDE.md

⚠️ Issues & Suggestions

1. Missing Dual-Mode Verification ⚠️ (CRITICAL per CLAUDE.md Rule 1)

Per CLAUDE.md Rule 1: ALL changes must work identically in both execution modes (source-generated AND reflection).

Question: Has this been tested in both modes?

• ✅ The code handles both paths (lines 126-143): cached attributes (source-gen) and reflection
• ❌ But: The tests don't explicitly verify both modes

Recommendation: Add explicit dual-mode tests to verify behavior is identical in both execution modes.

2. Potential Edge Case: Empty Data Sources 🤔

In GetParameterValues() at line 185-188, you check if values are empty after processing all attributes. But what if one attribute produces values and another produces none?

Current behavior: The combined list would still have values (correct)
Potential issue: The error message "data sources produced no values" might be misleading if only one of multiple attributes is empty

Suggestion: Consider logging which specific attribute produced no values, or verify this is the desired behavior.

3. Test Execution Missing ⚠️

Per CLAUDE.md's "TUnit.TestProject Testing Rules":

DO NOT run TUnit.TestProject without filters! Many tests are intentionally designed to fail.

Question: Were these tests actually executed to verify they pass?

Recommendation: Run with filter to verify tests pass and generate the expected number of test cases.

4. Minor: Code Comment Accuracy 📝

Line 224 comment says "Same algorithm as Matrix" but doesn't reference where Matrix is defined. Consider adding a reference to MatrixDataSourceAttribute for clarity.

🔍 Code Quality Assessment

Performance ✅

• No allocations in hot paths introduced
• Efficient Cartesian product using LINQ (acceptable for test setup, not runtime)
• Caching of attributes supported (lines 126-129)

Security ✅

• No security concerns
• Proper validation and error handling
• No user input directly executed

Maintainability ✅

• Clear separation of concerns
• Well-documented edge cases
• Good error messages aid debugging

📋 Pre-Commit Checklist Status

Based on CLAUDE.md requirements:

• ✅ Code follows style guide (modern C# syntax, braces, etc.)
• ✅ Public API snapshots updated (.verified.txt files committed)
• ✅ Error handling implemented
• ⚠️ Dual-mode testing: Not explicitly verified in tests
• ⚠️ Tests executed: Need confirmation tests actually run and pass
• ✅ No breaking changes (additive feature)
• ✅ AOT-compatible (uses cached attributes + reflection fallback)

🎯 Recommendations Before Merge

High Priority:

1. Verify tests pass: Run with proper filter and confirm all 5 test classes generate the expected number of test cases
2. Add dual-mode test: Explicitly test both source-gen and reflection modes

Medium Priority:

3. Consider enhancing error message to specify which attribute produced no values (if multiple attributes on same parameter)

Low Priority:

4. Add reference comment for Cartesian product algorithm

📊 Overall Assessment

Rating: ⭐⭐⭐⭐☆ (4/5)

This is a solid PR that solves the reported issue effectively with:

• ✅ Excellent error messaging
• ✅ Comprehensive test scenarios
• ✅ Clean implementation
• ✅ Proper snapshot updates

Main concern: Need confirmation of dual-mode behavior per TUnit's core requirements.

Recommendation: Approve with minor changes - verify tests pass and consider adding explicit dual-mode test coverage.

Great work on the improved error messaging! This will help users understand the limitation clearly. 🎉